Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug] fix missing discover context icon #1545

Merged
merged 1 commit into from
May 6, 2022
Merged

[Bug] fix missing discover context icon #1545

merged 1 commit into from
May 6, 2022

Conversation

joshuarrrr
Copy link
Member

Add back icon directive, which injects SVG for EUI icons

Signed-off-by: Josh Romero [email protected]

Description

Corrects rendering of discover context table row expansion buttons. The directive appears to have been removed when partially de-angularizing discover, without realizing the impact on the context doc table rendering.

Issues Resolved

fixes #1281

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@joshuarrrr
[Bug] fix missing discover context icon
3971bf5 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
@joshuarrrr joshuarrrr requested a review from a team as a code owner May 3, 2022 22:11
tmarkley
tmarkley approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Is it worthwhile to add tests to ensure the icons are now loaded properly to prevent a regression in the future?

@joshuarrrr
Copy link
Member Author

Nice! Is it worthwhile to add tests to ensure the icons are now loaded properly to prevent a regression in the future?

Ideally, yes, absolutely.

The problem is I'm not sure there's a straightforward way to do that for these legacy views that are rendered with a mix of Angular and React. Specifically, the regression case we'd want to test is that we're dynamically inserting the icon SVG on any angular-defined <icon> state change so that

<icon ng-if="open" type="'arrowDown'" size="'s'" class="ng-scope"></icon>

becomes

<icon ng-if="open" type="'arrowDown'" size="'s'" class="ng-scope"><svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" class="euiIcon euiIcon--small" focusable="false" role="img" aria-hidden="true"><path fill-rule="non-zero" d="M13.069 5.157L8.384 9.768a.546.546 0 01-.768 0L2.93 5.158a.552.552 0 00-.771 0 .53.53 0 000 .759l4.684 4.61c.641.631 1.672.63 2.312 0l4.684-4.61a.53.53 0 000-.76.552.552 0 00-.771 0z"></path></svg></icon>

It probably makes more sense to spend some time investigating the functional tests in discover, to see if they provide enough coverage for the larger effort of finishing the migration from Angular to React.

kavilla
kavilla approved these changes May 4, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

   __   _____________  ___
  / /  / ___/_  __/  |/  /
 / /__/ (_ / / / / /|_/ / 
/____/\___/ /_/ /_/  /_/

@tmarkley
Copy link
Contributor

tmarkley commented May 6, 2022

It probably makes more sense to spend some time investigating the functional tests in discover, to see if they provide enough coverage for the larger effort of finishing the migration from Angular to React.

@joshuarrrr I think that's reasonable. We'll need to address that with #1560.

@tmarkley tmarkley merged commit bd10227 into opensearch-project:main May 6, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
@joshuarrrr @github-actions
[Bug] fix missing discover context icon (#1545)
af59029 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 6, 2022
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
@joshuarrrr @github-actions
[Bug] fix missing discover context icon (#1545)
5b60cfb 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 6, 2022
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
@joshuarrrr @github-actions
[Bug] fix missing discover context icon (#1545)
5637a22 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 6, 2022
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
@joshuarrrr @github-actions
[Bug] fix missing discover context icon (#1545)
903f82a 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request May 6, 2022
Merged
tmarkley pushed a commit that referenced this pull request May 6, 2022
@opensearch-trigger-bot
[Bug] fix missing discover context icon (#1545) (#1564)
f63019d 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
@tmarkley tmarkley mentioned this pull request May 6, 2022
Closed
5 tasks
tmarkley pushed a commit that referenced this pull request May 10, 2022
@opensearch-trigger-bot
[Bug] fix missing discover context icon (#1545) (#1562)
78071c9 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
tmarkley pushed a commit that referenced this pull request May 10, 2022
@opensearch-trigger-bot
[Bug] fix missing discover context icon (#1545) (#1565)
5c00c1e 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
@tmarkley tmarkley mentioned this pull request May 10, 2022
Merged
1 task
joshuarrrr added a commit to joshuarrrr/OpenSearch-Dashboards that referenced this pull request May 11, 2022
@joshuarrrr
[Bug] fix missing discover context icon (opensearch-project#1545)
cafac37 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
joshuarrrr added a commit that referenced this pull request Jun 2, 2022
@joshuarrrr
[Bug] fix missing discover context icon (#1545)
d87c99b 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
tmarkley pushed a commit that referenced this pull request Jun 3, 2022
@opensearch-trigger-bot
[Bug] fix missing discover context icon (#1545) (#1563)
90ff204 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit bd10227)
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 8, 2022
@joshuarrrr @kavilla
[Bug] fix missing discover context icon (opensearch-project#1545)
c8989cd 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 16, 2022
@joshuarrrr @kavilla
[Bug] fix missing discover context icon (opensearch-project#1545)
e6e4ad8 
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boktorbb boktorbb boktorbb approved these changes

@kavilla kavilla kavilla approved these changes

@tmarkley tmarkley tmarkley approved these changes

Assignees
No one assigned
Labels
backport 1.3 backport 2.x bug Something isn't working v1.3.3 v2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Document table expand icon is invisible on Discover Context page
4 participants
@joshuarrrr @tmarkley @kavilla @boktorbb